AO3-7397 AO3-6765 Configurable page for skin previews#5744
AO3-7397 AO3-6765 Configurable page for skin previews#5744nicolacleary wants to merge 9 commits into
Conversation
nicolacleary
force-pushed
the
AO3-7397_skin_preview_page
branch
from
779a5ad to
c92baf3
Compare
| flash[:notice] << t(".remove_skin") | ||
| flash[:notice] << t(".tip") | ||
| flash[:notice] << ("<a href='#{skin_path(@skin)}' class='action' role='button'>".html_safe + t(".return_to_skin") + "</a>".html_safe) | ||
| redirect_to "#{ArchiveConfig.SKIN_PREVIEW_URL}?site_skin=#{@skin.id}" |
There was a problem hiding this comment.
This provides no guarantees that the url is correct (formatting or exists) - e.g. if you configure "/idonotexist" then you would get an error only when you tried to preview a skin.
There was a problem hiding this comment.
That's fine, but thanks for noting it!
Bilka2
left a comment
Bilka2
left a comment
There was a problem hiding this comment.
This is a full review now, but it's just one thing :D
| flash[:notice] << t(".remove_skin") | ||
| flash[:notice] << t(".tip") | ||
| flash[:notice] << ("<a href='#{skin_path(@skin)}' class='action' role='button'>".html_safe + t(".return_to_skin") + "</a>".html_safe) | ||
| redirect_to "#{ArchiveConfig.SKIN_PREVIEW_URL}?site_skin=#{@skin.id}" |
There was a problem hiding this comment.
That's fine, but thanks for noting it!
| flash[:notice] << t(".skin_title", title: @skin.title) | ||
| flash[:notice] << t(".remove_skin") | ||
| flash[:notice] << t(".tip") | ||
| flash[:notice] << ("<a href='#{skin_path(@skin)}' class='action' role='button'>".html_safe + t(".return_to_skin") + "</a>".html_safe) |
There was a problem hiding this comment.
Could you use helpers.link_to to create this link HTML so that we can avoid the html_safe call? If you need to remove the role='button' for that, that's fine, it should be removed per AO3-6765 anyway.
There was a problem hiding this comment.
Speaking of AO3-6765, the PR for it has been in "Reviewed: Action Needed" without changes for over a year, which makes it adoptable for AD&T volunteers like me. However, if I adopted it now, we'd have loads of merge conflicts between the changes here and the changes for AO3-6765.
Would you be up for adding AO3-6765 into the PR here, since you're already doing the i18n it should just be a matter of changing the text in the locale file and changing the construction of the flash here to not be an array (looks like we usually do multiline flashes with <br />)? If yes, you have my official AD&T blessing to adopt AO3-6765
There was a problem hiding this comment.
I'll look into using the helper as per your first comment.
As for AO3-6765 - I am happy to add that functionality into this PR or a separate one - seems like it's not that complicated in terms of what would change.
I am, however, not quite sure what the procedure is for this. I would rather avoid using the old PR since it has a lot of unrelated linting changes and, given its age, it would take some effort to bring up to date with master (of course it can be done though if necessary) .
Would it be okay to just recreate the code changes in a new commit and update the PR title and description as necessary to add the additional ticket name and credit?
There was a problem hiding this comment.
Would it be okay to just recreate the code changes in a new commit and update the PR title and description as necessary to add the additional ticket name and credit?
Yup!
github-actions
Bot
added
Coder Has Actioned Review
and removed
Reviewed: Action Needed
labels
nicolacleary
changed the title
Some extra tests were added to master in AO3-7388 that enforced the old behaviour (redirecting to a random tag for previewing a site skin) Updated so that they check for the value from config.yml '/users/OTW_Translation/works'
in site skin preview. This removes role 'button' but this will be removed as part of AO3-6765 anyways.
Adopt changes from PR otwcode#4934: - Adjust skin preview message text - Include skin id in preview message - Update text on return to skin link (role=button removed in a previous commit)
Use <br /> to form the multiline flash message for skin previews, rather than an array which turns it into an unordered list with <ul>
nicolacleary
force-pushed
the
AO3-7397_skin_preview_page
branch
from
ad3ba78 to
1a37970
Compare
|
The force push was to correct the Jira ticket in some of the commit messages since I got them mixed up. From what I can tell it is not recommended to do this after a review but I decided it would be alright since I was only targeting commits that hadn't been reviewed yet. |
| t(".skin_title", title: @skin.title), | ||
| t(".remove_skin"), | ||
| t(".tip", site_skin_id: @skin.id), | ||
| helpers.link_to(t(".return_to_skin"), skin_path(@skin), class: "action") | ||
| ].join('<br />') |
There was a problem hiding this comment.
Changes in flash message:
Details
Flash before:
Flash using <br /> rather than <ul>:
Flash using <br /> rather than <ul> and wrapping it in <p>:
AO3-6765 says:
The “Return To Skin To Use” link ... should ideally be in a
<p>, not a<li>. The role="button" can also be removed, because it is not acting like a button, just a link.
AFAIK I could just do view_context.content_tag(:p, <rest of the line>) and it would work (see screenshots above), but it's a little unclear for me what the desired look of this flash is, given that there are quite a few changes here now. I decided to leave it is as now since it's no longer part of a list.
| t(".remove_skin"), | ||
| t(".tip", site_skin_id: @skin.id), | ||
| helpers.link_to(t(".return_to_skin"), skin_path(@skin), class: "action") | ||
| ].join('<br />') |
There was a problem hiding this comment.
As far as I can tell, a .html_safe doesn't impact what is rendered in this case, so I omitted it.
3 participants
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-7397
https://otwarchive.atlassian.net/browse/AO3-6765 (adopted)
Purpose
Redirect to the URL specified in the config with
SKIN_PREVIEW_URLwhen previewing a skin.Remove the functionality to redirect to the works index of a random tag - this provided no guarantees on the kind of content shown and would sometimes choose tags that no longer existed (resulting in 500 errors).
This PR also adopts changes from #4934 to improve the skin preview message:
Credit
nicolacleary (she/her)
Adopted changes from AO3-6765 #4934 David Bilsky/Ironskink (He/Him)